Implement #7718 (Exposing hitboxes and their bounds) #32
Conversation
Having model hitboxes be this accessible allows us to easily target and modify specific ones such as increasing the size of the head hitbox on lower game difficulties or removing hitboxes entirely on "dismembered" enemies. |
Expose ModelHitboxes component's Hitboxes list Expose Hitbox's bounds
|
This seems mostly fine to me. I don't like the fact you can add and remove hitboxes but AddHitbox is already public here but that may have been a mistake. |
handsomematt
added
the
triaged
There was a problem hiding this comment.
Pull request overview
This PR implements issue #7718 by exposing hitboxes and their bounds to the public API. It adds methods to retrieve and manipulate hitboxes in ModelHitboxes, exposes the Bounds property on Hitbox, and makes the ModelChanged event public on ModelRenderer.
Changes:
- Added methods to query hitboxes by bone index, bone, bone name, and physics body
- Exposed the
Boundsproperty onHitboxfrom internal to public - Made
ModelChangedevent onModelRendererpublic with documentation - Added
HitboxesRebuiltevent to notify when hitboxes are rebuilt
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| engine/Sandbox.Engine/Scene/Components/Render/ModelRenderer.cs | Made ModelChanged event public and added XML documentation |
| engine/Sandbox.Engine/Scene/Components/Game/ModelHitboxes.cs | Added GetHitboxes, RemoveHitbox, and overloaded GetHitbox methods; added HitboxesRebuilt event; refactored properties to use field keyword |
| engine/Sandbox.Engine/Scene/Components/Game/Hitbox.cs | Changed Bounds property visibility from internal to public |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| GameObject _target; | ||
|
|
||
| /// <summary> | ||
| /// The target GameObject to report in trace hits. If this is unset we'll defaault to the gameobject on which this component is. |
There was a problem hiding this comment.
Corrected spelling of 'defaault' to 'default'.
| /// The target GameObject to report in trace hits. If this is unset we'll defaault to the gameobject on which this component is. | |
| /// The target GameObject to report in trace hits. If this is unset we'll default to the gameobject on which this component is. |
| internal readonly List<Hitbox> Hitboxes = new(); | ||
|
|
||
| /// <summary> | ||
| /// Adds a hitbox to the models hitbox list |
There was a problem hiding this comment.
The XML documentation should use possessive form. Change 'models' to 'model's'.
| } | ||
|
|
||
| /// <summary> | ||
| /// Removes a hitbox from the models hitbox list. |
There was a problem hiding this comment.
The XML documentation should use possessive form. Change 'models' to 'model's'.
| /// <param name="hitbox"></param> | ||
| public void RemoveHitbox( Hitbox hitbox ) | ||
| { | ||
| Hitboxes.Remove( hitbox ); |
There was a problem hiding this comment.
RemoveHitbox does not call Dispose() on the removed hitbox, which could lead to a resource leak. The Hitbox class implements IDisposable and has a Dispose method that removes the PhysicsBody. Consider calling hitbox.Dispose() after removing it from the list, similar to how Clear() handles removal.
| Hitboxes.Remove( hitbox ); | |
| if ( Hitboxes.Remove( hitbox ) ) | |
| { | |
| hitbox?.Dispose(); | |
| } |
5 participants
Added GetHitboxes and RemoveHitbox to ModelHitboxes
Expose Hitbox's bounds
Thank you